Add proper gas limit check through local computation #14707
Conversation
| // else: | ||
| // gas_diff = parent_gas_limit - target_gas_limit | ||
| // return parent_gas_limit - min(gas_diff, max_gas_limit_difference) | ||
| func expectedGasLimit(parentGasLimit, targetGasLimit uint64) uint64 { |
There was a problem hiding this comment.
targetGasLimit may be a bit confusing given that we have "gas limit" and "gas target" already mean something.
There was a problem hiding this comment.
Renamed it to proposerGasLimit. Same as lighthouse
| // gas_diff = parent_gas_limit - target_gas_limit | ||
| // return parent_gas_limit - min(gas_diff, max_gas_limit_difference) | ||
| func expectedGasLimit(parentGasLimit, targetGasLimit uint64) uint64 { | ||
| maxGasLimitDiff := parentGasLimit/gasLimitAdjustmentFactor - 1 |
There was a problem hiding this comment.
This has an underflow if parentGasLimit < 1024, can't happen in real life though
| return parentGasLimit + maxGasLimitDiff | ||
| } | ||
| return targetGasLimit | ||
| } else { |
There was a problem hiding this comment.
No need for an else branch here.
terencechain
force-pushed
the
check-header-gas-limit
branch
from
7065b2d to
77abdfd
Compare
| reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx) | ||
| if err != nil { | ||
| log.WithError(err).Warn("Proposer: failed to get registration by validator ID, could not check gas limit") | ||
| } else { | ||
| gasLimit := expectedGasLimit(parentGasLimit, reg.GasLimit) | ||
| if gasLimit != header.GasLimit() { | ||
| return nil, fmt.Errorf("incorrect header gas limit %d != %d", gasLimit, header.GasLimit()) | ||
| } | ||
| } |
There was a problem hiding this comment.
You don't seem to need this else either:
| reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx) | |
| if err != nil { | |
| log.WithError(err).Warn("Proposer: failed to get registration by validator ID, could not check gas limit") | |
| } else { | |
| gasLimit := expectedGasLimit(parentGasLimit, reg.GasLimit) | |
| if gasLimit != header.GasLimit() { | |
| return nil, fmt.Errorf("incorrect header gas limit %d != %d", gasLimit, header.GasLimit()) | |
| } | |
| } | |
| reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx) | |
| if err != nil { | |
| log.WithError(err).Warn("Proposer: failed to get registration by validator ID, could not check gas limit") | |
| } | |
| gasLimit := expectedGasLimit(parentGasLimit, reg.GasLimit) | |
| if gasLimit != header.GasLimit() { | |
| return nil, fmt.Errorf("incorrect header gas limit %d != %d", gasLimit, header.GasLimit()) | |
| } |
There was a problem hiding this comment.
I think you need the else, we dont want to call expectedGasLimit if error
There was a problem hiding this comment.
reg is used so if there' s an error the call to expectedGasLimit can' t be carried
| @@ -243,6 +248,16 @@ func (vs *Server) getPayloadHeaderFromBuilder(ctx context.Context, slot primitiv | |||
| return nil, fmt.Errorf("incorrect parent hash %#x != %#x", header.ParentHash(), h.BlockHash()) | |||
| } | |||
|
|
|||
| reg, err := vs.BlockBuilder.RegistrationByValidatorID(ctx, idx) | |||
There was a problem hiding this comment.
Does the registration cache handle the case where the validator does not include a value?
There was a problem hiding this comment.
I think so. I went through the code as my understanding: the DefaultBuilderGasLimit is initially set in config.go:
It is then defined as 30M in mainnet_config.go:
The beacon node does not provide a flag to set this gas limit, and it is not part of the consensus spec configuration. Instead, it is hard-coded in the client. The DefaultBuilderGasLimit is also defined as a default in the validator's flag.go. If the validator does not specify the --suggested-gas-limit flag, the value in mainnet_config.go overrides it:
The DefaultBuilderGasLimit is used in proposer_setting.go's getProposerSettings function. This function displays or recreates the currently applied proposer settings under the validator's custom flags:
In loader.go, the DefaultBuilderGasLimit ensures that the default gas limit applies if the --suggested-gas-limit flag is not set. It is then saved to the proposer settings at startup when registering for the validator node service:
The SaveProposerSettings function is called within the Load method of settingsLoader in loader.go. This step saves the default gas limit to the proposer settings file:
When registering the validator client, the system ensures that the proposer settings with the correct default gas limit are stored in the database:
There was a problem hiding this comment.
Not sure I get it right but it seems there is something confusing here, when using --proposer-settings-file at the validator level, if in the file if there is:
{
"default_config": {
"builder": {
"enabled": false,
"gas_limit": "42"
},
"fee_recipient": "XXXX"
},
"my_key": {
"YYYY": {
"builder": {
"enabled": true
"gas_limit": "43",
},
"fee_recipient": "YYYYY"
},
When using --suggested-fee-recipient=ZZZZ, my_key will still propose on YYYY (proposer settings file has priority over flag).
However, when using --suggested-gas-limit=10, my_key will use the flag override instead of 43 (flag has priority for gas limit over proposer settings).
In processBuilderConfig, gas limit is overridden by the flag while the fee recipient is not:
Also, if the user specifies for my_key a gas limit but no fee-recipient, it seems no fee-recipient is used? (or maybe later in the hot-path it defaults to the flag?).
It could make sense to align both behavior (to always have the file override the flags).
There was a problem hiding this comment.
yes there's a bug there, similar bug: #14721
unrelated to this PR though so will merge this PR and fix that today
This PR adds gas limit check through local computation and compare expected gas limit against builder's header. The expected gas limit is the following with
adjustment_factorset to 1024